Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move v2 metadata models to ecs-agent module #3705

Merged
merged 2 commits into from
May 22, 2023
Merged

Move v2 metadata models to ecs-agent module #3705

merged 2 commits into from
May 22, 2023

Conversation

amogh09
Copy link
Contributor

@amogh09 amogh09 commented May 18, 2023 •
edited
Loading

Summary

Move v2 metadata models to ecs-agent module. These models will be needed in ecs-agent module in future work when we move metadata endpoint handlers to ecs-agent module.

Implementation details

  • Move TaskResponse, ContainerResponse, LimitsResponse, and ErrorResponse structs from agent/handlers/v2/response.go to ecs-agent/tmds/handlers/v2/response.go.
  • Add a new struct HealthStatus for metadata health status that replaces apicontainer.HealthStatus from agent/api/container package in ContainerResponse. This is done to decouple metadata health status from API Container health status that is used in a lot of places. The new struct has the same fields except for Status field that has type string instead of ContainerHealthStatus enum.
  • A new dockerContainerHealthToV2Health function is added to agent/handlers/v2/response.go that converts apicontainer.HealthStatus to metadata HealthStatus. TestDockerContainerHealthToV2HealthJSON is added to ensure that the JSON representations of the two structs is the same.
  • Existing unit tests in agent/handlers/v2/response_test.go are updated with new asserts for health status for greater confidence in the new health status struct.

Testing

New unit tests are added for the new health status struct.

In addition to automated tests, also performed manual tests to ensure that the v2, v3, and v4 metadata endpoints return the same metadata with and without the changes in this PR.

New tests cover the changes: yes

Description for the changelog

Move v2 metadata models to ecs-agent module

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@amogh09 amogh09 changed the base branch from master to dev May 18, 2023 22:01
@amogh09 amogh09 force-pushed the move-v2-models branch 3 times, most recently from e166b21 to 51d3dbd Compare May 18, 2023 23:15
amogh09
amogh09 commented May 18, 2023
View reviewed changes
ecs-agent/tmds/handlers/v2/response.go
}

// Container health status
type HealthStatus struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new type which matches the apicontainer.HealthStatus type that is being used in v2 metadata response currently.

amazon-ecs-agent/agent/api/container/container.go

Lines 111 to 120 in 883211e

type HealthStatus struct {
// Status is the container health status
Status apicontainerstatus.ContainerHealthStatus `json:"status,omitempty"`
// Since is the timestamp when container health status changed
Since *time.Time `json:"statusSince,omitempty"`
// ExitCode is the exitcode of health check if failed
ExitCode int `json:"exitCode,omitempty"`
// Output is the output of health check
Output string `json:"output,omitempty"`
}

This is being done to decouple metadata response models from API models.

@amogh09 amogh09 marked this pull request as ready for review May 19, 2023 01:33
@amogh09 amogh09 requested a review from a team as a code owner May 19, 2023 01:33
chienhanlin
chienhanlin reviewed May 19, 2023
View reviewed changes
agent/handlers/v2/response.go
func dockerContainerHealthToV2Health(health apicontainer.HealthStatus) *tmdsv2.HealthStatus {
status := health.Status.String()
if health.Status == apicontainerstatus.ContainerHealthUnknown {
// Skip sending status if it is unknown
Copy link
Contributor

@chienhanlin chienhanlin May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q. did we previously send heath status ContainerHealthUnknown? If yes, would this be a new change introduced to v2? Also curious how would Since, ExitCode and Output look like when status is ContainerHealthUnknown : )

Copy link
Contributor Author

@amogh09 amogh09 May 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question. No we do not send Status when container's health status is ContainerHealthUnknown because the Status field in apicontainer.HealthStatus struct is defined with json:"status,omitempty".

ContainerHealthUnknown is the zero value of Status's type so Status = ContainerHealthUnknown is skipped when apicontainer.HealthStatus is marshaled to JSON.

The TestDockerContainerHealthToV2HealthJSON test in this PR covers this case.

amazon-ecs-agent/agent/api/container/container.go

Lines 111 to 120 in 883211e

type HealthStatus struct {
// Status is the container health status
Status apicontainerstatus.ContainerHealthStatus `json:"status,omitempty"`
// Since is the timestamp when container health status changed
Since *time.Time `json:"statusSince,omitempty"`
// ExitCode is the exitcode of health check if failed
ExitCode int `json:"exitCode,omitempty"`
// Output is the output of health check
Output string `json:"output,omitempty"`
}

amazon-ecs-agent/agent/api/container/status/containerstatus.go

Lines 43 to 50 in 883211e

const (
// ContainerHealthUnknown is the initial status of container health
ContainerHealthUnknown ContainerHealthStatus = iota
// ContainerHealthy represents the status of container health check when returned healthy
ContainerHealthy
// ContainerUnhealthy represents the status of container health check when returned unhealthy
ContainerUnhealthy
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining this in detail!

@amogh09 amogh09 merged commit 5f9ccdb into aws:dev May 22, 2023
@ubhattacharjya ubhattacharjya mentioned this pull request May 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chienhanlin chienhanlin chienhanlin approved these changes

@ubhattacharjya ubhattacharjya ubhattacharjya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amogh09 @ubhattacharjya @chienhanlin @amazon-ecs-bot